Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Permissions for access requests #2242

Merged
merged 1 commit into from
Jul 14, 2023
Merged

Permissions for access requests #2242

merged 1 commit into from
Jul 14, 2023

Conversation

max-moser
Copy link
Contributor

@max-moser max-moser commented May 31, 2023 •
edited
Loading

Update regarding the structure of this PR

This PR has been split into three different PRs, more details are available in the comment below.

Overview

This PR requires inveniosoftware/invenio-rdm-records#1306 to work.
It provides a UI for the access requests detail pages, as well as a button for requesting access from restricted records' landing pages.

To do

  • UI for the guest-based access requests
  • New share modal according to the RFC
    • Management of access grants
    • Management of secret links
    • Management of record access settings

Some thoughts

Users should be able to remove access grants that give themselves access to the record (i.e. where the current user is the subject), but the UI should warn them beforehand ("are you really sure? this might lock you out").

@max-moser max-moser linked an issue Jun 1, 2023 that may be closed by this pull request
Access requests #2180
Closed
1 task
@max-moser max-moser mentioned this pull request Jun 1, 2023
Merged
kpsherva
kpsherva reviewed Jun 1, 2023
View reviewed changes
invenio_app_rdm/records_ui/templates/semantic-ui/invenio_app_rdm/records/detail.html Outdated
@@ -252,6 +252,21 @@ <h2 id="files-heading">{{ _('Files') }}</h2>
<p>{{ _("Reason") }}: {{ record.access.embargo.reason }}</p>
{% endif %}

{%- if not current_user.is_anonymous %}
{# TODO allow a similar workflow for guests #}
<hr style="border-color: darkred;">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

styling: in general we consider the inline styling as a bad practice (fe. some stricter content security policies will throw an error), we use the semantic-ui based theme for this, pure html: https://semantic-ui.com/
and react: https://react.semantic-ui.com/,
it will help with removing the multiple paragraphs if you use semantic ui headers and containers

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, this was just a very quick & dirty hack to make something work

kpsherva
kpsherva reviewed Jun 1, 2023
View reviewed changes
invenio_app_rdm/requests_ui/templates/semantic-ui/invenio_requests/access-request/index.html Outdated
)
}}

<div>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above, semantic ui classes are better fit here :)

kpsherva
kpsherva reviewed Jun 1, 2023
View reviewed changes
invenio_app_rdm/requests_ui/views/requests.py Outdated
@@ -137,6 +137,19 @@ def user_dashboard_request_view(request, **kwargs):
request_is_accepted=request_is_accepted,
)

else:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ping @anikachurilova I think this will be modified already in your PR, could you confirm ?

Copy link
Contributor

@anikachurilova anikachurilova Jun 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to me that render template of is_draft_submission and is_record_inclusion can be used here as well as I used it for Upgrade Legacy Record request (https://github.com/zenodo/zenodo-rdm/pull/354/files#diff-4d09ad616277a10edaf2a5b5a11eef4e1aebd90e1ab43f95e2b7114c8eeacf88R14).
No need to add a new one
@max-moser check here how adjusted the renders by topics (feel free to review and comment if something seems wrong :)) https://github.com/inveniosoftware/invenio-app-rdm/pull/2225/files#diff-4a94629a771abc41b540cb59d363d886634610dd06924aad8305ccae9250ced7R104

kpsherva
kpsherva reviewed Jun 1, 2023
View reviewed changes
invenio_app_rdm/theme/assets/semantic-ui/js/invenio_app_rdm/landing_page/index.js Outdated

const axiosWithConfig = axios.create(apiConfig);
const recid = requestAccessButton.dataset.recid;
const result = await axiosWithConfig.post(`/api/records/${recid}/access/request`, {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this a new endpoint? because we already have api/records/<recid>/requests reachable inside the record link, ie. record.links.requests, should we reuse that one ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, POST /api/records/<recid>/access/requests and POST /api/records/<recid>/access/requests/guest are the new endpoints for creating access requests.

kpsherva
kpsherva reviewed Jun 1, 2023
View reviewed changes
invenio_app_rdm/theme/assets/semantic-ui/js/invenio_app_rdm/landing_page/index.js Outdated

// TODO it would be nicer to have the backend report the proper `links.self_html`
// instead of constructing the URL here
const url = `/me/requests/${id}`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed, we need rest API to give us the link :)

@max-moser max-moser force-pushed the mm/access-requests branch 2 times, most recently from c3f750c to 8654b02 Compare June 5, 2023 08:09
max-moser
max-moser commented Jun 29, 2023
View reviewed changes
invenio_app_rdm/config.py Outdated
@@ -1177,3 +1178,7 @@ class NotificationsUserSchema(UserSchema):

NOTIFICATIONS_SETTINGS_VIEW_FUNCTION = notification_settings
"""View function for notification settings."""

from invenio_rdm_records.services.permissions import RDMRequestsPermissionPolicy
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is required because we need to tweak the requests permission policy slightly for guest-based access requests

@max-moser
Copy link
Contributor Author

max-moser commented Jul 3, 2023 •
edited
Loading

This PR includes:

  • The access request form on the record landing pages for both user-based and guest-based workflows
    • They are currently implemented as React components, but it should be easy enough to turn most of it into more static Jinja
  • Initial work for reworking the share modal according to the RFC
    • Displaying a list of all access grants for the record already works
    • Changing permissions for individual access grants also works
    • Supports display of several shared links now, rather than just "the first" shared link for the record

Some functionality is still missing:

  • Creation of access grants (this requires a search bar for users, plus selection of permissions)
  • Management of shared links (this fell victim to the rework, and hasn't been reimplemented)
  • The entire "access request settings" tab

@max-moser max-moser force-pushed the mm/access-requests branch 3 times, most recently from d3b67c7 to 0d15e57 Compare July 6, 2023 10:13
@max-moser
Copy link
Contributor Author

Update

This PR has been split into three separate PRs, dealing with...

  1. Setting the request permission policy to make access requesets work (this PR)
  2. Adding the frontend pieces required to make the access requests usable by guests and users (Record landing pages: access request forms #2287)
  3. Reworking the share modal according to the access requests RFC (Rework the share modal #2286)

@max-moser max-moser changed the title Access requests: frontend part Permissions for access requests Jul 6, 2023
@max-moser max-moser force-pushed the mm/access-requests branch 2 times, most recently from b13a9a5 to 314fe5a Compare July 6, 2023 15:11
@max-moser
config: update config for access requests
820548e 
* set requests permission policy defined in rdm-records
* this is necessary to allow guests to access the guest request access
  details via the access request tokens
* also, schedule cleanup of expired access request tokens
@zzacharo zzacharo marked this pull request as ready for review July 13, 2023 16:00
@zzacharo zzacharo merged commit b4da01a into inveniosoftware:master Jul 14, 2023
12 checks passed
@max-moser max-moser deleted the mm/access-requests branch July 14, 2023 08:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kpsherva kpsherva kpsherva left review comments

@anikachurilova anikachurilova anikachurilova left review comments

@zzacharo zzacharo zzacharo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Access requests
4 participants
@max-moser @zzacharo @kpsherva @anikachurilova